Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable enqueuing updates for expired items when serving stale #13

Merged

Conversation

simonbos
Copy link
Contributor

Currently, the logic behaves like the following when the ServeStale is true.

|______________________ ____threshold window__________ ______________ ____threshold window x2________ ______expiry & no threshold__________|
0 min                   9 mins                         10 mins        11 mins
Add key here           Update                          Update         Update                          No Update

I adapted it to also enqueue an update in the expired after the second "threshold window".

Please let me know if the previous behavior was on purpose or if it was a bug. If it was not purpose, I will also create a test case.

@bnkamalesh bnkamalesh added the bug Something isn't working label Nov 26, 2024
@bnkamalesh
Copy link
Member

bnkamalesh commented Nov 26, 2024

hey @simonbos thank you for that, it indeed looks like a miss, good catch. My tests weren't testing this scenario as well, it only checked if servestale worked, that too without an updater.

I'll wait for your test cases update before merging.

@simonbos simonbos force-pushed the enable-enqueueing-update-stale-expired branch from 8e8dff1 to e15b7f9 Compare November 26, 2024 10:18
@simonbos
Copy link
Contributor Author

Thanks! I confirmed that TestThresholdUpdaterStale/long after threshold (cache expired) failed on the main branch.

Copy link
Member

@bnkamalesh bnkamalesh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@bnkamalesh bnkamalesh enabled auto-merge (squash) November 26, 2024 11:09
@bnkamalesh bnkamalesh merged commit 0be51f5 into naughtygopher:main Nov 26, 2024
1 check passed
@simonbos simonbos deleted the enable-enqueueing-update-stale-expired branch November 26, 2024 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants